West-Midlands | 26-ITP-May | Maryam Janjua | Sprint 3 | Sprint 3 TDD Coursework - #1604
maryam-devio wants to merge 6 commits into
Conversation
maryam-devio
added
🐂 Size Medium
hackertainment
added
Review in progress
There was a problem hiding this comment.
The given example of aaaaa is a special case. There should be more general cases for multiple occurences.
Also, there is one more boundary case for no occurrences. Can you think of it?
hackertainment
left a comment
hackertainment
left a comment
There was a problem hiding this comment.
You generally got the right direction and just need to clear the logic a bit. Good work and keep it up :-)
| function getOrdinalNumber(num) { | ||
| return "1st"; | ||
| let getStr = num.toString(); | ||
| let lastTwoDigit = getStr.slice(-2); |
There was a problem hiding this comment.
while this line works even when the string only contains a single digit, it would be hard to maintain in a more complex project.
There was a problem hiding this comment.
There is no test case for the lastTwoDigit part of your function. Please add them back. Thanks.
There was a problem hiding this comment.
Please tidy-up the indentation so that it would be easier to read. It is also a good and important practice as well.
| } | ||
| return result; | ||
| } | ||
| else if (count == 0){ |
| // The goal is to re-implement that function, not to use it. | ||
| return "hellohellohello"; | ||
| let result=""; | ||
| if (count >=1){ |
There was a problem hiding this comment.
How about empty string (with the same set of counts 3, 1, 0, -1)?
hackertainment
added
Reviewed
|
Hello, thank you for your valuable feedback. I learned from all the comments you made. I've updated my code and pushed the changes. Thanks again! |
There was a problem hiding this comment.
Since the updated version directly using number for processing, the if and else if would better be using number comparison as well. Especially when you use === instead of ==, you will see what I meant.
p.s.: in the future, please label it as "Needs review" (and remove "Reviewed") for re-review. Otherwise the system will not show up your PR after your fixes. Thank you.
There was a problem hiding this comment.
Hi! I understand now. I've updated the comparisons to use numbers and ===, Thank you!
maryam-devio
added
Needs Review
|
Congratulation, this PR is completed :-) |
hackertainment
added
Complete
|
Closing PR because the May ITP run has finished. Feel free to re-open if you're still working on it. |
|
Your PR didn't include a Task ID in its description. Make sure you have put the correct Task ID in its description. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). If this PR needs reviewed, please add the 'Needs Review' label to this PR after you have resolved the issues listed above. |
4 participants
Learners, PR Template
Self checklist
Changelist
I have attempt all the TDD coursework according to the requirments and test all the changes.